-
Notifications
You must be signed in to change notification settings - Fork 294
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Require Encodable
body for all GET
requests
#1166
Conversation
httpRequestWithCaching(method: "GET", path: path, parameters: parameters, completion: completion) | ||
} else { | ||
httpRequest(method: "GET", path: path, parameters: parameters, completion: completion) | ||
func get(_ path: String, parameters: Encodable? = nil, shouldCache: Bool = false, completion: @escaping RequestCompletion) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a future PR, we can make parameters
non-optional, since there are no get()
calls in the SDK that don't include this value.
XCTAssertEqual(lastGetParameters["currencyIsoCode"] as! String, "fake-code") | ||
XCTAssertEqual(lastGetParameters["paymentMethodNonce"] as! String, "fake-nonce") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This work has been a good exercise, since it reveals that we aren't testing the formatting of most our JSON bodies for both GET & POST requests right now 😬 (as also evidenced in #1151)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't completely understand the example that reveals we aren't testing the formatting of JSON bodies. Maybe understanding it would give me the answer to my question.
Q: Why do we care about the formatting of the JSON as long as the key value pairs are the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks great! just one curious q: what would be the main benefit for using Encodable
in the request body
Great question!
|
/// The GET parameters for `v1/payment_methods/amex_rewards_balance` | ||
struct BTAmexRewardsBalanceRequest: Encodable { | ||
|
||
private let currencyIsoCode: String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit for a future PR but we should update this to currencyISOCode
to match our style guide preferences. Same in the init
Summary of changes
BTHTTP.get()
method to requireEncodable
body instead of raw dictionary paramGET
requests that exist in the SDKBTAmexRewardsBalanceRequest
BTConfigurationRequest
Checklist
Added a changelog entryAuthors
@scannillo